Aggregation improvements #10965

skrzypo987 · 2022-02-07T15:26:37Z

Few neat tricks to speed up aggregation a bit

sopel39 · 2022-02-07T16:56:00Z

fyi @martint

sopel39 · 2022-02-07T17:00:26Z

@skrzypo987 could you share perf results?

skrzypo987 · 2022-02-08T09:05:39Z

@skrzypo987 could you share perf results?

I messed up some things so there is a regression. Will get back with the results once I fix it.

skrzypo987 · 2022-02-08T10:07:38Z

before:

BenchmarkGroupByHashOnTpch.groupBy                  false        0  avgt   20    9,134 ±  0,555  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false        1  avgt   20   66,969 ±  2,017  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false      2_0  avgt   20   23,235 ±  0,689  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false      2_1  avgt   20   40,110 ±  0,420  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false        3  avgt   20   55,257 ±  1,738  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false      4_0  avgt   20   48,726 ±  1,106  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false      4_1  avgt   20   81,607 ±  0,805  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false        5  avgt   20   53,240 ±  1,124  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false        7  avgt   20  123,469 ±  1,859  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false        8  avgt   20    8,932 ±  0,136  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false        9  avgt   20   60,057 ±  1,000  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false     11_0  avgt   20   22,098 ±  0,386  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false     11_1  avgt   20   12,275 ±  0,140  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false       12  avgt   20   47,678 ±  1,636  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false     13_0  avgt   20   27,317 ±  0,579  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false     13_1  avgt   20   37,461 ±  0,424  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false       15  avgt   20   22,595 ±  0,226  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false     16_0  avgt   20  533,342 ±  4,900  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false     16_1  avgt   20  257,857 ±  4,977  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false       17  avgt   20   25,246 ±  0,836  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false       18  avgt   20   82,138 ±  1,520  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false       20  avgt   20   35,989 ±  1,407  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false       21  avgt   20  385,947 ± 10,523  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false       22  avgt   20   24,116 ±  0,744  ns/op

after

BenchmarkGroupByHashOnTpch.groupBy                  false        0  avgt   20    6,756 ±  0,049  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false        1  avgt   20    2,521 ±  0,034  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false      2_0  avgt   20   19,771 ±  0,672  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false      2_1  avgt   20   39,528 ±  0,643  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false        3  avgt   20   47,519 ±  1,084  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false      4_0  avgt   20   48,953 ±  0,771  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false      4_1  avgt   20   69,582 ±  1,493  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false        5  avgt   20   51,688 ±  1,756  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false        7  avgt   20  127,517 ±  0,964  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false        8  avgt   20    6,692 ±  0,035  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false        9  avgt   20   65,012 ±  1,214  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false     11_0  avgt   20   17,419 ±  0,184  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false     11_1  avgt   20   12,352 ±  0,160  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false       12  avgt   20   48,045 ±  0,949  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false     13_0  avgt   20   22,845 ±  0,593  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false     13_1  avgt   20   35,829 ±  0,841  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false       15  avgt   20   19,283 ±  0,421  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false     16_0  avgt   20  487,442 ±  5,271  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false     16_1  avgt   20  212,259 ±  4,008  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false       17  avgt   20   22,631 ±  0,952  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false       18  avgt   20   70,313 ±  1,561  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false       20  avgt   20   23,919 ±  1,027  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false       21  avgt   20  362,150 ± 10,634  ns/op
BenchmarkGroupByHashOnTpch.groupBy                  false       22  avgt   20   21,731 ±  0,560  ns/op

skrzypo987 · 2022-02-09T08:52:37Z

I got rid of the biggest commit as it showed some regressions that were difficult to spot. I'll return to it in different PR.
The only benchmark change with the current state of the PR is tpch/q01 going from ~50ns/row to ~2.5ns/row

core/trino-main/src/main/java/io/trino/operator/MultiChannelGroupByHash.java

lukasz-stec · 2022-02-15T08:45:55Z

core/trino-main/src/main/java/io/trino/operator/MultiChannelGroupByHash.java

+                    combinationIds[i] = (short) blocks[0].getId(i);
+                }
+                for (int j = 1; j < channels.length; j++) {
+                    for (int i = 0; i < positionCount; i++) {


why iterate over channels first? we then update combinations multiple times but we could compute it once per position

So that we iterate over a single block at a time. Microbenchmarks show better results that way.

lukasz-stec · 2022-02-15T09:54:00Z

core/trino-main/src/main/java/io/trino/operator/MultiChannelGroupByHash.java

+                }
+                for (int j = 1; j < channels.length; j++) {
+                    for (int i = 0; i < positionCount; i++) {
+                        combinationIds[i] *= dictionarySizes[channels.length - j];


is this correct that we take dict channels.length - j?
Im not sure if I understand correctly but e.g.

dict size 1, 2, 3 values 0, 1, 0 (((0 * 2) + 1 ) * 2) + 0 = 2 values 0, 0, 2 (((0 * 2) + 0 ) * 4) + 2 = 2

You are absolutely right. This is one of those "how could that even worked" kind of bug.
I fixed it and added some tests.

core/trino-main/src/main/java/io/trino/operator/MultiChannelGroupByHash.java

skrzypo987 · 2022-02-21T09:41:15Z

Got rid of the commit introducting tpch aggregation benchmark as its newer version exists in #11031

core/trino-main/src/main/java/io/trino/operator/MultiChannelGroupByHash.java

sopel39 · 2022-02-23T14:44:40Z

core/trino-main/src/main/java/io/trino/operator/MultiChannelGroupByHash.java

@@ -219,6 +223,9 @@ public void appendValuesTo(int groupId, PageBuilder pageBuilder, int outputChann
        if (isRunLengthEncoded(page)) {


you should call page = page.getLoadedPage() before dictionary checks (same for getGroupIds). Same for BigintGroupByHash

This is unrelated to the PR

core/trino-main/src/test/java/io/trino/operator/TestGroupByHash.java

sopel39 · 2022-02-23T14:53:59Z

Is BenchmarkGroupByHashOnTpch part of OS? These benchmark results should be part of commit message

core/trino-main/src/main/java/io/trino/operator/MultiChannelGroupByHash.java

sopel39 · 2022-02-25T14:25:10Z

core/trino-main/src/main/java/io/trino/operator/MultiChannelGroupByHash.java

+                return false;
+            }
+            cardinality = multiplyExact(cardinality, ((DictionaryBlock) page.getBlock(channel)).getDictionary().getPositionCount());
+            if (cardinality > positionCount * SMALL_DICTIONARIES_MAX_CARDINALITY_RATIO || cardinality > SMALL_DICTIONARIES_MAX_CARDINALITY) {


nit: I would remove cardinality > SMALL_DICTIONARIES_MAX_CARDINALITY and only leave ratio unless there is strong reason to keep this check

Fine. Made some benchmarks and there is indeed little chance for regression

core/trino-main/src/main/java/io/trino/operator/MultiChannelGroupByHash.java

core/trino-main/src/test/java/io/trino/operator/TestGroupByHash.java

sopel39

lgtm % comments

core/trino-main/src/main/java/io/trino/operator/MultiChannelGroupByHash.java

sopel39 · 2022-03-01T16:44:15Z

core/trino-main/src/test/java/io/trino/operator/TestGroupByHash.java

+        secondBlock = BlockAssertions.createLongDictionaryBlock(10, 100, 7);
+        page = new Page(firstBlock, secondBlock);
+
+        groupByHash.addPage(page).process();


core/trino-main/src/test/java/io/trino/operator/TestGroupByHash.java

sopel39

lgtm % commnent

core/trino-main/src/main/java/io/trino/operator/MultiChannelGroupByHash.java

If the number of combinations of all dictionaries in a page is below certain number, we can store the results in a small array and reuse found groups

cla-bot bot added the cla-signed label Feb 7, 2022

skrzypo987 requested review from lukasz-stec and sopel39 February 7, 2022 15:26

Remove warnings

301ff47

skrzypo987 force-pushed the skrzypo/053-aggregation-improvements branch from 8ea23c9 to 2cb6340 Compare February 8, 2022 09:04

skrzypo987 added the WIP label Feb 8, 2022

skrzypo987 force-pushed the skrzypo/053-aggregation-improvements branch 2 times, most recently from eb12136 to f5ae9a7 Compare February 9, 2022 08:51

skrzypo987 removed the WIP label Feb 9, 2022

skrzypo987 force-pushed the skrzypo/053-aggregation-improvements branch from f5ae9a7 to 406a41b Compare February 9, 2022 15:12

lukasz-stec reviewed Feb 15, 2022

View reviewed changes

skrzypo987 force-pushed the skrzypo/053-aggregation-improvements branch 3 times, most recently from 7589e02 to 25eca66 Compare February 21, 2022 09:39

sopel39 reviewed Feb 23, 2022

View reviewed changes

skrzypo987 force-pushed the skrzypo/053-aggregation-improvements branch 2 times, most recently from 8d185a4 to ef28b26 Compare February 25, 2022 12:50

sopel39 reviewed Feb 25, 2022

View reviewed changes

skrzypo987 force-pushed the skrzypo/053-aggregation-improvements branch 3 times, most recently from e9ed1ba to 1e161fa Compare March 1, 2022 07:29

skrzypo987 requested a review from sopel39 March 1, 2022 11:09

sopel39 reviewed Mar 1, 2022

View reviewed changes

skrzypo987 force-pushed the skrzypo/053-aggregation-improvements branch from 1e161fa to 59f8dfb Compare March 4, 2022 12:52

skrzypo987 requested a review from sopel39 March 4, 2022 12:53

sopel39 approved these changes Mar 4, 2022

View reviewed changes

core/trino-main/src/main/java/io/trino/operator/MultiChannelGroupByHash.java Outdated Show resolved Hide resolved

skrzypo987 force-pushed the skrzypo/053-aggregation-improvements branch from 59f8dfb to f78d524 Compare March 7, 2022 07:05

sopel39 reviewed Mar 7, 2022

View reviewed changes

core/trino-main/src/main/java/io/trino/operator/MultiChannelGroupByHash.java Outdated Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/operator/MultiChannelGroupByHash.java Show resolved Hide resolved

Add low cardinality dictionary aggregation

ffd1ee8

If the number of combinations of all dictionaries in a page is below certain number, we can store the results in a small array and reuse found groups

skrzypo987 force-pushed the skrzypo/053-aggregation-improvements branch from f78d524 to ffd1ee8 Compare March 7, 2022 11:10

skrzypo987 requested a review from sopel39 March 7, 2022 12:35

sopel39 approved these changes Mar 7, 2022

View reviewed changes

sopel39 merged commit 82b4c47 into trinodb:master Mar 7, 2022

sopel39 mentioned this pull request Mar 7, 2022

Release notes for 373 #11288

Closed

github-actions bot added this to the 373 milestone Mar 7, 2022

mosabua mentioned this pull request Mar 7, 2022

Add Trino 373 release notes #11290

Merged

pettyjamesm mentioned this pull request Apr 4, 2022

Query Fails when distinct and limit operators are present #11776

Closed

takezoe mentioned this pull request Sep 8, 2022

Multi channel group by hash first batch of optimizations treasure-data/presto#54

Merged

fgwang7w mentioned this pull request Apr 18, 2023

Improve performance of aggregation operator prestodb/presto#19425

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aggregation improvements #10965

Aggregation improvements #10965

skrzypo987 commented Feb 7, 2022

sopel39 commented Feb 7, 2022

sopel39 commented Feb 7, 2022

skrzypo987 commented Feb 8, 2022

skrzypo987 commented Feb 8, 2022

skrzypo987 commented Feb 9, 2022

lukasz-stec Feb 15, 2022

skrzypo987 Feb 16, 2022

lukasz-stec Feb 15, 2022

skrzypo987 Feb 16, 2022

skrzypo987 commented Feb 21, 2022

sopel39 Feb 23, 2022

skrzypo987 Feb 24, 2022

sopel39 commented Feb 23, 2022

sopel39 Feb 25, 2022

skrzypo987 Feb 28, 2022

sopel39 left a comment

sopel39 Mar 1, 2022

sopel39 left a comment

		@@ -219,6 +223,9 @@ public void appendValuesTo(int groupId, PageBuilder pageBuilder, int outputChann
		if (isRunLengthEncoded(page)) {

Aggregation improvements #10965

Aggregation improvements #10965

Conversation

skrzypo987 commented Feb 7, 2022

sopel39 commented Feb 7, 2022

sopel39 commented Feb 7, 2022

skrzypo987 commented Feb 8, 2022

skrzypo987 commented Feb 8, 2022

skrzypo987 commented Feb 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skrzypo987 commented Feb 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sopel39 commented Feb 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sopel39 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sopel39 left a comment

Choose a reason for hiding this comment